OCPBUGS-65690: Visiting Group Detail Page > RoleBindings will show error #15739
Conversation
…rray and object formats. Update tableFilters to ensure values.all is an array before processing. This improves filter robustness across components.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/valid-reference
|
@Leo6Leo: This pull request references Jira Issue OCPBUGS-65690, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Leo6Leo: This pull request references Jira Issue OCPBUGS-65690, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe PR normalizes filter handling across RBAC and table filter components to support both array and non-array input forms. It adds guards to ensure array checks before operations and converts single objects to arrays when needed, improving robustness against inconsistent filter input types. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@Leo6Leo: This pull request references Jira Issue OCPBUGS-65690, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Leo6Leo
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Leo6Leo, logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/public/components/RBAC/role.jsx (1)
286-294: Add explicit return for missing filter handlers.The
every()callback returnsundefinedwhenfiltersMap[filterKey]doesn't exist, causing the filter to fail and incorrectly exclude data.Apply this diff:
return data.filter((binding) => { return filtersArray.every((filter) => { const filterKey = Object.keys(filter)[0]; const filterValue = filter[filterKey]; // Use the table filter function if it exists if (filtersMap[filterKey]) { return filtersMap[filterKey](filterValue, binding); } + // If no filter exists for this key, pass the item through + return true; }); });frontend/public/components/RBAC/bindings.tsx (1)
304-312: Add explicit return for missing filter handlers.This has the same issue as role.jsx: the
every()callback returnsundefinedwhen a filter key is not found infiltersMap, causing valid data to be incorrectly excluded.Apply this diff:
return data.filter((binding) => { return filtersArray.every((filter) => { const filterKey = Object.keys(filter)[0]; const filterValue = filter[filterKey]; // Use the table filter function if it exists if (filtersMap[filterKey]) { return filtersMap[filterKey](filterValue, binding); } + // If no filter exists for this key, pass the item through + return true; }); });
🧹 Nitpick comments (1)
frontend/public/components/RBAC/bindings.tsx (1)
293-313: Consider extracting the staticFilters logic into a shared utility.The staticFilters normalization and filtering logic is duplicated between this file and role.jsx. Extracting it into a reusable function would improve maintainability.
Example approach:
// In a shared utilities file export const applyStaticFilters = (data, staticFilters, filtersMap) => { if (!staticFilters || !data) { return data; } const filtersArray = Array.isArray(staticFilters) ? staticFilters : [staticFilters]; return data.filter((item) => { return filtersArray.every((filter) => { const filterKey = Object.keys(filter)[0]; const filterValue = filter[filterKey]; if (filtersMap[filterKey]) { return filtersMap[filterKey](filterValue, item); } return true; }); }); };Then use it in both files:
const filteredData = React.useMemo(() => { return applyStaticFilters(data, staticFilters, tableFilters(false)); }, [data, staticFilters]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/public/components/RBAC/bindings.tsx(1 hunks)frontend/public/components/RBAC/role.jsx(1 hunks)frontend/public/components/factory/table-filters.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/RBAC/bindings.tsxfrontend/public/components/RBAC/role.jsxfrontend/public/components/factory/table-filters.ts
🔇 Additional comments (6)
frontend/public/components/RBAC/role.jsx (1)
282-283: LGTM! Normalization handles both array and object forms.The conversion correctly addresses the root cause of the "every is not a function" error by ensuring
staticFiltersis always treated as an array.frontend/public/components/RBAC/bindings.tsx (1)
300-301: LGTM! Consistent normalization approach.The staticFilters normalization correctly handles both array and object inputs, matching the implementation in role.jsx.
frontend/public/components/factory/table-filters.ts (4)
56-62: LGTM! Array guard prevents runtime errors.The array check correctly prevents calling
.every()on non-array values. Returningtrue(include item) when the filter is malformed is a reasonable default that avoids hiding data from users.
64-70: LGTM! Consistent array guard.The guard matches the pattern used in the alerts filter, maintaining consistency across the codebase.
72-75: LGTM! Defensive array check prevents errors.The inline array check ensures
.every()is only called on arrays. The short-circuit evaluation provides a safe fallback, consistent with the other filter guards.
99-105: LGTM! Completes the defensive pattern.The array guard follows the same pattern as the alerts and observe-rules filters, ensuring consistent error handling across all label-based filters.
|
The error log seems irrelevant to the code change in this PR. Might be flaky tests |
The cypress server is currently down. Hold for retesting. |
|
/retest |
|
/retest-required |
|
@Leo6Leo: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Description
Visiting Group Detail Page > RoleBindings will show error: XXX.every is not a function
How to reproduce?
This pr fixed the issue mentioned above.
Root Cause
The code tried to call .every() on an object, which caused the crash. Converting it to array and adding the check to return early if it's not an array solve the problem.